TST: Extend timedelta64 arithmetic tests to TimedeltaArray #23642
Conversation
…al code changes to make tests pass
|
Hello @jbrockmendel! Thanks for submitting the PR.
|
Codecov Report
@@ Coverage Diff @@
## master #23642 +/- ##
=========================================
Coverage ? 92.29%
=========================================
Files ? 161
Lines ? 51489
Branches ? 0
=========================================
Hits ? 47520
Misses ? 3969
Partials ? 0
Continue to review full report at Codecov.
|
gfyoung
added
Testing
| @@ -288,6 +317,21 @@ def _evaluate_with_timedelta_like(self, other, op): | |||
|
|
|||
| return NotImplemented | |||
|
|
|||
| __mul__ = _wrap_tdi_op(operator.mul) | |||
There was a problem hiding this comment.
we should use the already existing machinery and simply implement this _create_arithmetic_method
and a limited form of:
@classmethod
def _add_arithmetic_ops(cls):
otherwise you have different styles here than elsewhere.
There was a problem hiding this comment.
We use this pattern in plenty of places: nattype, Timedelta, field accessors TDA/DTA/PA, ... Using the classmethod in this context doesn't add anything except a layer of indirection.
There was a problem hiding this comment.
right but that'st the point, we should be using this here as this IS using the mixin that implements this. This is creating yet another pattern.
There was a problem hiding this comment.
as this IS using the mixin that implements this
No, it isn't. It inherits from ExtensionOpsMixin to use _add_comparison_comps, NOT _add_arithmetic_ops. In fact if were to try to use _add_arithmetic_ops that would be working at cross-purposes with _add_datetimelike_methods, which specifically implements add/sub-like methods.
I'm as big a fan of internal consistency and de-duplication as the next guy, but this isn't the place to make that stand.
There was a problem hiding this comment.
@jreback ok for now?
(anyway, I think the goal is to rework this a bit at some point, to not wrap TimedeltaIndex here, rather the other way around)
There was a problem hiding this comment.
I think the goal is to rework this a bit at some point, to not wrap TimedeltaIndex here, rather the other way around
Correct
| __rmul__ = __mul__ | ||
| __truediv__ = _wrap_tdi_op(operator.truediv) | ||
| __floordiv__ = _wrap_tdi_op(operator.floordiv) | ||
| __rfloordiv__ = _wrap_tdi_op(ops.rfloordiv) |
There was a problem hiding this comment.
how are you implmenting rfloordiv but not rdiv / rtruediv?
There was a problem hiding this comment.
very specifically implementing only the methods needed to extend the affected tests. I'll double-check why i couldn't extend tests for rdiv and rtruediv
There was a problem hiding this comment.
@jbrockmendel any findings here?
There was a problem hiding this comment.
AFAICT we don't have test cases for the other methods, at least not in tests.arithmetic.test_timedelta64. I've scoured the other test files to centralize arithmetic tests, but it's conceivable I've missed some.
We definitely need such tests, but I'd rather do that in a dedicated PR, and would prefer not to implement the methods here without the corresponding tests.
There was a problem hiding this comment.
We definitely need such tests, but I'd rather do that in a dedicated PR, and would prefer not to implement the methods here without the corresponding tests.
+1
| @@ -288,6 +317,21 @@ def _evaluate_with_timedelta_like(self, other, op): | |||
|
|
|||
| return NotImplemented | |||
|
|
|||
| __mul__ = _wrap_tdi_op(operator.mul) | |||
There was a problem hiding this comment.
@jreback ok for now?
(anyway, I think the goal is to rework this a bit at some point, to not wrap TimedeltaIndex here, rather the other way around)
| __rmul__ = __mul__ | ||
| __truediv__ = _wrap_tdi_op(operator.truediv) | ||
| __floordiv__ = _wrap_tdi_op(operator.floordiv) | ||
| __rfloordiv__ = _wrap_tdi_op(ops.rfloordiv) |
There was a problem hiding this comment.
@jbrockmendel any findings here?
| DatetimeArrayMixin as DatetimeArray) | ||
|
|
||
|
|
||
| def get_upcast_box(box, vector): |
There was a problem hiding this comment.
something similar is not needed in the other files in tests/arithmetic ?
There was a problem hiding this comment.
Eventually we'll want something like this, probably put it next to tm.box_expected. After this I'll end up rebasing #23734 and see if this can be re-used in the other files. There are enough special cases that it isn't obvious off the top of my head.
There was a problem hiding this comment.
did you rebase off of #23734? isn't this basically box_expected with another arg? there are a LOT of helpers out there. too many.....
There was a problem hiding this comment.
Yes this has been rebased. This is about determining what box to use when we are parametrizing over two box types. I agree it may be nice to cut down on the helpers; in fact I think we may be able to get rid of tm.box_expected if we're lucky (fixing the transpose thing...)
|
can you rebase |
|
Updated to use |
|
Following this I think there will be just one more round of DTA/TDA arithmetic test PRs. Almost there. |
The "one more" this was referring to was #23789, but I forgot one other: the follow-up discussed above to implement+test rdiv, rtruediv, etc. I'm pretty psyched to have this all wrapped up. |
|
gentle ping. follow-up waiting in the wings |
|
@jbrockmendel Thanks! |
…fixed * upstream/master: TST: Extend timedelta64 arithmetic tests to TimedeltaArray (pandas-dev#23642)
A handful of non-test changes to make the tests pass, all non-test changes are strictly necessary.
Some of the tests are not yet extended, as making the TDA case work will require more invasive changes.